-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make command-argument array public (in readonly
mode)
#1970
Conversation
Matches how command and option arrays are public. Useful for third-party libraries.
This fixes #1823
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid immediately breaking existing client code which may have directly accessed _args
, I suggest this._args = this.registeredArguments
in constructor (and deprecating _args
to remove later).
@@ -281,6 +281,9 @@ export class Command { | |||
processedArgs: any[]; | |||
readonly commands: readonly Command[]; | |||
readonly options: readonly Option[]; | |||
readonly registeredArguments: readonly Argument[]; | |||
/** @deprecated Use `.registeredArguments` instead. */ | |||
readonly _args: readonly Argument[]; // added here for strikethrough highlighting in editors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readonly _args: readonly Argument[]; // added here for strikethrough highlighting in editors | |
readonly _args: readonly Argument[]; |
@@ -281,6 +281,9 @@ export class Command { | |||
processedArgs: any[]; | |||
readonly commands: readonly Command[]; | |||
readonly options: readonly Option[]; | |||
readonly registeredArguments: readonly Argument[]; | |||
/** @deprecated Use `.registeredArguments` instead. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use same style as other deprecated messages
/** @deprecated Use `.registeredArguments` instead. */ | |
/** @deprecated since v11, instead use `.registeredArguments` */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually thought about specifying the version, but decided against it because the property had never been supported officially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks noisy because the name is so much longer! But not many authors will be typing it, and best name so far (and get away from the existing args/arguments/arguments). Should be accessible for symmetry with commands
and options
for people adding functionality.
Couple of minor changes for the comments adding and deprecating _args
to typings. (Which surprised me at first, but happy now.)
(I'll update deprecation documentation later to more closely follow match the style I am using.)
Problem
The array of registered command-arguments (currently
._args
) is not made publicly available unlike the arrays of registered options (.options
) and subcommands (.commands
). That is not only an obvious inconsistency, but also hinders third-party library development.Solution
Rename
._args
to.registeredArguments
(because.arguments
is already used for a method) and add the field to the type declaration forCommand
inreadonly
mode.Other names considered:
.commandArguments
: confusing since the word "command" is not included in the names of.argument()
,.arguments()
and.addArgument()
.addedArguments
: makes it seem like only arguments added vis.addArguments()
are includedThe use of the word "registered" is also consistent with the name of
._registerOption()
, an internal routine suggested in #1923 and #1931.ChangeLog
Added
.registeredArguments
(array withArgument
instances for all registered command-arguments)